Set AuthStyle to InParams for public PKCE OAuth clients by gkatz2 · Pull Request #4150 · stacklok/toolhive

gkatz2 · 2026-03-15T00:44:40Z

Summary

ToolHive's OAuth PKCE token exchange fails against strict OAuth 2.1 servers (e.g., Datadog's mcp.datadoghq.com) because oauth2.Endpoint structs default to AuthStyleAutoDetect, which probes with HTTP Basic Auth first and burns the single-use authorization code. See the linked issue for the full mechanism.

Set AuthStyle: oauth2.AuthStyleInParams in the three locations where oauth2.Endpoint is constructed without it
Add a regression test with a strict mock server that rejects Basic Auth for public clients

Fixes #4149

Type of change

Test plan

Unit tests (task test)
Linting (task lint-fix)
Manual testing: Verified OAuth PKCE flow completes successfully against Datadog's MCP server (mcp.datadoghq.com)

Changes

File	Change
`pkg/auth/oauth/flow.go`	Set `AuthStyleInParams` on endpoint in `NewFlow`
`pkg/auth/remote/handler.go`	Set `AuthStyleInParams` on endpoint in `tryRestoreFromCachedTokens`
`pkg/registry/auth/oauth_token_source.go`	Set `AuthStyleInParams` on endpoint in `buildOAuth2Config`
`pkg/auth/oauth/flow_test.go`	Add `TestAuthStyleInParams_StrictPublicClientServer` regression test

Does this introduce a user-facing change?

Yes. Remote MCP servers with strict OAuth 2.1 implementations that previously failed with invalid_grant during PKCE token exchange will now connect successfully. No configuration changes needed.

Special notes for reviewers

Why AuthStyleInParams is safe: AuthStyleAutoDetect tries HTTP Basic Auth first. For public PKCE clients (the common case — ToolHive's DCR registers with token_endpoint_auth_method=none), this sends an Authorization: Basic base64(client_id:) header with an empty password. Spec-compliant servers reject this and consume the single-use authorization code, making the retry fail with invalid_grant. AuthStyleInParams sends client_id in the POST body instead, avoiding the problem entirely.

For the less common case where users provide their own client credentials via --remote-auth-client-id/--remote-auth-client-secret, AuthStyleInParams sends the secret in the POST body (client_secret_post style). This is accepted by the vast majority of OAuth servers. The only servers that would break are those that exclusively require client_secret_basic and reject POST body credentials — an uncommon configuration, and a much smaller risk than the auth-code-burning bug this fixes.

Generated with Claude Code

When oauth2.Endpoint.AuthStyle is unset (zero value), Go's oauth2 library uses AuthStyleAutoDetect, which tries HTTP Basic Auth first. For public PKCE clients (token_endpoint_auth_method=none), this sends an Authorization header with an empty password. Spec-compliant servers reject this and consume the single-use authorization code, causing the retry with client_id in POST body to fail with invalid_grant. Set AuthStyleInParams explicitly in all three locations where oauth2.Endpoint is constructed without AuthStyle: - pkg/auth/oauth/flow.go (authorization code exchange) - pkg/auth/remote/handler.go (token refresh from cached tokens) - pkg/registry/auth/oauth_token_source.go (registry auth) Add regression test with a strict mock server that rejects Basic Auth for public clients. Without the fix: 2 requests (auto-detect probing). With the fix: exactly 1 request. Fixes stacklok#4149 Signed-off-by: Greg Katz <gkatz@indeed.com>

Signed-off-by: Greg Katz <gkatz@indeed.com>

codecov · 2026-03-15T03:44:54Z

Codecov Report

❌ Patch coverage is 53.33333% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.86%. Comparing base (0de510d) to head (67e42e1).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/remote/handler.go	0.00%	6 Missing ⚠️
pkg/auth/oauth/flow.go	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4150      +/-   ##
==========================================
- Coverage   69.13%   68.86%   -0.27%     
==========================================
  Files         462      462              
  Lines       46554    46572      +18     
==========================================
- Hits        32185    32073     -112     
- Misses      11897    11910      +13     
- Partials     2472     2589     +117

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

amirejaz

Thanks for the well-documented fix — the root cause analysis is accurate and the test structure is the right approach. Two issues need addressing before merge:

AuthStyleInParams is hardcoded for confidential clients in flow.go and handler.go. Both paths can have a non-empty ClientSecret (e.g., DCR-registered clients in handler.go). Forcing InParams will break any OAuth server that mandates client_secret_basic. The fix should be conditional on whether ClientSecret is empty.
Data race in the regression test: requestCount is accessed from two goroutines without synchronisation — the test goroutine reads it while the httptest.Server goroutine writes it. go test -race will flag this.

The change in pkg/registry/auth/oauth_token_source.go looks correct — that path is always a public-client PKCE flow (no secret).

- Make AuthStyle conditional on ClientSecret: public clients (no secret) use AuthStyleInParams to avoid burning single-use auth codes on strict servers; confidential clients use AuthStyleAutoDetect so servers that mandate client_secret_basic are not broken - Fix data race in regression test: replace plain int requestCount with atomic.Int32 so the httptest.Server goroutine and test goroutine access it safely under -race Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

amirejaz · 2026-03-16T15:45:26Z

Hey @gkatz2, great fix! I pushed a small follow-up commit to your branch to address the conditional AuthStyle for confidential clients and a data race in the test.

gkatz2 requested review from ChrisJBurns, JAORMX, jhrozek, rdimitrov and yrobla as code owners March 15, 2026 00:44

github-actions bot added the size/S Small PR: 100-299 lines changed label Mar 15, 2026

Fix gci lint in flow_test.go

0b50b34

Signed-off-by: Greg Katz <gkatz@indeed.com>

github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Mar 15, 2026

theJC mentioned this pull request Mar 16, 2026

Preserve query params in remote MCP server URLs #4154

Merged

4 tasks

amirejaz requested changes Mar 16, 2026

View reviewed changes

Comment thread pkg/auth/remote/handler.go Outdated

Comment thread pkg/auth/oauth/flow.go Outdated

Comment thread pkg/auth/oauth/flow_test.go Outdated

github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Mar 16, 2026

Merge branch 'main' into fix/oauth-authstyle-inparams

67e42e1

github-actions bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Mar 16, 2026

reyortiz3 approved these changes Mar 16, 2026

View reviewed changes

amirejaz approved these changes Mar 17, 2026

View reviewed changes

amirejaz merged commit a7d1c98 into stacklok:main Mar 17, 2026
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set AuthStyle to InParams for public PKCE OAuth clients#4150

Set AuthStyle to InParams for public PKCE OAuth clients#4150
amirejaz merged 4 commits intostacklok:mainfrom
gkatz2:fix/oauth-authstyle-inparams

gkatz2 commented Mar 15, 2026 •

edited

Loading

Uh oh!

codecov bot commented Mar 15, 2026 •

edited

Loading

Uh oh!

amirejaz left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amirejaz commented Mar 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gkatz2 commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

Changes

Does this introduce a user-facing change?

Special notes for reviewers

Uh oh!

codecov bot commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

amirejaz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amirejaz commented Mar 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gkatz2 commented Mar 15, 2026 •

edited

Loading

codecov bot commented Mar 15, 2026 •

edited

Loading